ci: run full test suite on Windows - #652
Conversation
|
I have the enumeration this draft is waiting for — I ran the full suite on Windows locally while working on #641/#644/#645/#646/#647, so here it is up front rather than after a red shard. windows/amd64, go1.26.6,
|
| cluster | n |
|---|---|
TestLoadCompileCommands_* (clangd compile DB) |
5 |
TestGDScript* |
5 |
TestTSConfigPathAlias_* |
3 |
symlink confinement (*SymlinkOutOfRepo*) |
4 |
| startup-barrier / marker permission fail-closed | 2 |
| unclustered | 13 |
internal/mcp — 27
| cluster | n | note |
|---|---|---|
| symlink / file-mode | 5 | SeCreateSymbolicLinkPrivilege, and Windows has no POSIX mode bits |
| review rulepack join | 4 | 3 addressed by #646, 1 is a different cause |
| localization ranking (Swift ×3, Rust, C#, PHP) | 6 | not diagnosed |
| fidelity glob | 1 | #647 |
| POSIX-spelled assertions | ~4 | e.g. TestPathToFileURI_Absolute wants file:///work/main.go, Windows resolves file:///D:/work/main.go |
| unclustered | ~7 |
Two things that will bite the shard, and one is the same class as the failure you already hit
1. Some local Windows failures are the developer's environment, not the platform. Your note about .claude/worktrees/* and TestNewIsFencedToIndexerStaging is exactly this class. I hit my own: TestMapGitDiffRepoPrefixJoin and TestMapGitDiffFileChangeKinds fail here on TempDir RemoveAll cleanup: …\.git: The directory is not empty — because the git on my PATH is a non-stock fork that writes .git/ai/logs into every git init. Reproduced with a bare git init in an empty directory outside any repo. A clean runner with stock git will not show those two, so my analysis count of 4 should read as 2 for CI purposes.
Worth stating in the workflow or the contributing notes that the Windows shard is authoritative and a local red is suspect until reproduced there — the inverse of the usual advice, and non-obvious.
2. fail-fast: false plus a full suite will surface order-dependent flakes, and there is at least one. TestNotesManager_SaveQueryDelete passed in one full-package run and failed 3 of 3 when run in isolation on the same commit. I nearly credited a fix for it in #647 on the strength of a before/after count. Anyone reading the first red shard should diff failing test names against a baseline rather than counts, or a flake will read as a regression — and vice versa.
On removing the build-windows job
No objection from me, and the reasoning in #647 is fair — a selector list is a guess and mine did drift. One consequence worth deciding deliberately rather than inheriting: those steps also carried three non-test guarantees that the full-suite matrix does not reproduce — the CGO/tree-sitter build smoke test (go build ./... on native Windows), and the gortex.exe CLI build. If the full shard only runs go test, a Windows build break lands without a red until someone runs the suite. Cheap to keep as two steps in the new shard if you want that signal.
Happy to take any of the clusters above once the shard has enumerated them for real — the .NET/GDScript/TSConfig ones in particular look like one root cause each rather than per-test work.
Addresses the review on zzet#646. reviewChangedGraphPaths inferred "this path is already graph-keyed" from strings.HasPrefix(f, repoPrefix+"/"). The two domains overlap, so that is not recoverable by inspection: in a repo whose tree carries a top-level directory named like the repo prefix, `repo-a/pkg/widget.go` is a valid git-relative path AND a valid graph key for a different file. The inference skips the real key `repo-a/repo-a/pkg/widget.go`, so the changed file is never scanned - and when a same-named `pkg/widget.go` exists, that unchanged shadow is scanned in its place. Make the domain explicit and travel with the data: changedPathsRepoRelative git's spelling, relative to the working tree changedPathsGraphKeyed the graph's "<prefix>/<rel>" node key All four production callers pass DiffResult.ChangedFiles, which MapGitDiff documents as keeping "the diff-relative paths (callers re-join them with git pathspecs)", so they pass changedPathsRepoRelative and the prefix is now applied unconditionally. Only the test covering a caller that already holds node keys passes changedPathsGraphKeyed. The Norm-based join and output normalization from the first revision are unchanged; this only replaces the inference. Regression test: a fixture carrying both pkg/widget.go and repo-a/pkg/widget.go, only the nested path marked changed, asserting every match reports the changed target. Both files carry the detector fixture, so a wrong-target scan still returns matches and only the reported path separates the outcomes - restoring the HasPrefix inference fails it with "pkg/widget.go" is the unchanged shadow. The ci.yml selector hunk is dropped: zzet#652 removes that job outright.
|
Full classification, as promised. Measured on 173 failing tests across 28 packages — but 2 of those are my machine, not the repo, so read the number as 171.
Buckets
Buckets 1–6 are individually enumerated (I have the test names). Bucket 7 is the residual, classified by first error line, so a handful may move as each is opened. Bucket 7 is the same family as #646 and it reaches production code, not just fixtures. Confirmed samples:
Three things I need from you1. File modes (bucket 3) — a real decision, and one is security-shaped.
2. The launchd / XDG tests. 3. Sequencing. The Windows leg cannot go green in one step — 171 failures across 26 packages is several PRs of work, and one PR carrying all of it would not be reviewable. Two ways to land it:
I would take (a). It reports real numbers from day one, it does not ask anyone to maintain a selector list you have already said can miss the regression it is meant to protect, and the flip to blocking becomes a one-line PR at the end. I have not measured bucket 2 on your runner. GitHub's windows image may well hold #655 is the first tranche and is independent of whichever you pick. |
|
Correction to my own numbers above, before they mislead anyone. The baseline I measured, Re-measured on current main
#646 closed 3 in each — the three So the headline is 167 failing tests, not 173, and 165 after subtracting the two that are my machine. The bucket shape is unchanged: the six I enumerated are unaffected, and the correction comes entirely out of bucket 7, which drops from ≈115 to ≈109. Everything else in that comment stands, including the three questions. #655 is unaffected — it was measured against |
|
@tiendungdev - if you need any help with Windows tests let me know. |
479da4e to
c81c1af
Compare
…ndent Three defects that only surface on a Windows checkout, found while enumerating the failures for the windows-latest matrix leg (zzet#652). 1. No .gitattributes, so a Windows checkout rewrites every text file to CRLF (Git for Windows installs with core.autocrlf=true, the GitHub windows runner included). Two consequences: - `internal/agents/opencode/plugin/gortex.js` and `internal/agents/pi/extension/index.ts` are go:embed'd and written verbatim into a user's project, so a source build on Windows ships CRLF assets. TestPluginFailsOpen already catches this: it splits pluginSource on "\n}\n", which no longer matches. - `gofmt -l` flags every Go file in the tree, so a formatting gate on the Windows leg fails wholesale and a Windows developer cannot tell real findings from the noise. The committed blobs are already LF — `git add --renormalize .` reports no content change — so this pins the checkout and rewrites nothing. 2. GlobalPointerBody built the @-include with filepath.Join, embedding `@C:\Users\me\.gortex\instructions\active.md` into ~/.claude/CLAUDE.md. That line is document content, not a filesystem call, and every other path in the same file is '/'-spelled. shellSafeHookBinary already normalises for exactly this reason. UpsertMarkedBlock keys idempotency on the markers, not on the path, so an existing install has its block rewritten in place on the next run. 3. normalizeRender scrubbed only the native spelling of HOME, the repo root and the resolved gortex binary, but a rendered manifest carries '/'-spelled paths by design. On Windows the substitution missed, so TestAgentsRenderGolden leaked a machine-specific absolute into the comparison and drifted for any developer with gortex on PATH. Verification (windows/amd64, go1.26.6, -count=1): internal/agents ok (was ok) internal/agents/opencode 1 -> 0 failures cmd/gortex 19 -> 18 failures internal/agents/claudecode 2 -> 2 failures (both pre-existing: TestResolveHookCommand, TestEmitPluginBundle_HookHandlerExecBit) Newly-broken set is empty. Each fix was sabotage-verified on its own: reverting (1) fails TestPluginFailsOpen, reverting (2) drifts the claude-code golden, reverting (3) drifts claude-code and hermes. The two updated assertions built their expected @-include with filepath.Join, which asserts the native mangling on Windows and passes there whether or not the renderer normalises; they now use path.Join / filepath.ToSlash. As in zzet#646, none of this can fail on the linux/macos matrix — filepath.ToSlash is a no-op on POSIX — so the Windows runner is the only place these bind.
3 participants
Summary
windows-latestto the existing full test matrix used by Linux and macOSfail-fast: falseso every OS shard completesbuild-windowsjobWhy
PR #647 showed that focused Windows test-name selectors can miss the exact regression they are meant to protect. Running
go test -race -timeout=30m -coverprofile=coverage.out ./...on every supported OS makes the platform contract explicit and lets CI reveal portability failures directly.macOS was already running the same full command as Linux; this change brings Windows into that matrix.
Validation
actionlint .github/workflows/ci.ymlgo build -o <temp>/gortex ./cmd/gortex/The repository-wide local test was attempted in the shared checkout. Its only observed failure was
TestNewIsFencedToIndexerStaging, caused by unrelated.claude/worktrees/*directories being scanned. GitHub clean runners are the authoritative result.Draft status
The previous workflow deliberately avoided the full Windows suite, and recent Windows runs documented existing platform-assumption failures. This PR stays draft while the new shard enumerates those failures so they can be classified from concrete CI output rather than another guessed selector list.